Don't propagate errors into Node.js - #2926
Merged
Merged
Conversation
node:events:505
throw er; // Unhandled 'error' event
^
Error: too hot to handle!
at $c_Lfs2_io_internal_ThrowableOps$ThrowableOps.toJSError__sjs_js_Error (/workspace/fs2/io/js/target/scala-2.13/fs2-io-test-fastopt/https:/raw.githubusercontent.com/typelevel/fs2/9e924ca24ef72d15b09825905ea29b344e4bd67b/io/js/src/main/scala/fs2/io/internal/ThrowableOps.scala:30:34)
at /workspace/fs2/io/js/target/scala-2.13/fs2-io-test-fastopt/https:/raw.githubusercontent.com/typelevel/fs2/9e924ca24ef72d15b09825905ea29b344e4bd67b/io/js/src/main/scala/fs2/io/ioplatform.scala:74:41
diesalbla
approved these changes
Jun 26, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
H/t @satabin for reporting in gnieh/fs2-data#335 (comment).
Previously, exceptions encountered in
fs2.Streamwould also be propagated into the Node.js stream it was interopping with. In v3.2.8 this started crashing the Node.js process with:(Note: the
unexpected '1' before object keyis an expected error raised in the fs2-data test suite.)Propagating the error seems sensible, but probably is not the right thing to do.
I think this was an existing bug, that became exposed through the changes in #2918. Specifically in 0c71749 besides replacing the facade I swapped in the
MicrotaskExecutorforSyncIO(probably should have done it in another PR :) This introduced a newcede/yielddue to theevalOnwhich gave the error event an opportunity to emit when previously it did not.